fix: various debug fixes and VS Code compatibility enhancements #11984

kittaakos · 2022-12-13T12:02:40Z

What it does

feat: added support for debug/toolbar and debug/variables/context,
feat: added support for debugState when context (vscode: add support for debug/toolbar, debug/variables/context menus contribution and debugState when clause context #11871),
feat: can customize debug session timeout, and error handling ([debug]: Cannot stop debug session of vscode-mock-debug #11879),
fix: the debugType context that was not updated on session stop,
fix: configure must happen after receiving capabilities ([debug]: Debug session might be configured with an empty capabilities object #11886),
fix: added missing conext menu in the Variables view,
fix: handle setFunctionBreakboints response with no body ([debug] Theia does not stop on breakpoints with the debugger provided by the vscode.mock-debug extension #11885),
fix: DebugExt fires didStart event on didCreate ([debug] debug-ext prematurely fires didStart event on didCreate #11916),
fix: validate editor selection based on the text model ([debug] Error: Illegal value for lineNumber when "stopOnEntry": true in the launch.json #11880)

Closes #11871
Closes #11879
Closes #11885
Closes #11886
Closes #11916
Closes #11880

How to test

Get the modified version of the vscode.mock-debug extension from my fork: https://github.com/kittaakos/vscode-mock-debug/blob/theia/mock-debug-0.51.0.vsix. Here is the diff with the changes from the upstream version: microsoft/vscode-mock-debug@main...kittaakos:vscode-mock-debug:theia.
Open the ./vscode-mock-debug/sampleWorkspace folder as a workspace in Theia. There is a readme file in it and the launch.json in the .vscode folder.
Press F5, readme.md opens, and the top stack frame is the first line of the readme file. Check the DevTools console; there is no Error: Illegal value for lineNumber. (#11880)

debug_001.mp4

Set a breakpoint before any variable declarations (line 17) and after (line 28). You can use other lines too. Continue the debugger. It hits the breakpoint. (#11885). You see no local variable. Continue the debugger. It hits the next breakpoint when there are variables. It's visible from the Variables view. Right-click the variable, and you will see the debugType and debugState dependent content menu from the VSIX. Click on it. It works. You also see two additional debug toolbar items—one with the ❤️ codicon and the other with text. Click on them, and both work. (#11871)

debug_002.mp4

For reviewers:

fix: DebugExt fires didStart event on didCreate ([debug] debug-ext prematurely fires didStart event on didCreate #11916),

This 👆 requires a more complex setup and another VSIX (marus25.cortex-debug-1.5.1.vsix from https://downloads.arduino.cc/marus25.cortex-debug/marus25.cortex-debug-1.5.1.vsix). The fix cannot be verified with the mock-debug extension. Also, I used a physical board (Arduino Zero) where the code runs. The code runs on the device, and the debugger talks to it via dgb. I am happy to provide the steps to set up the toolchain, but the board is needed. Let me know it's required.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

packages/debug/src/browser/view/debug-toolbar-widget.tsx

kittaakos · 2023-01-06T08:15:34Z

Hi, is there any update on this PR? Can somebody please help with the review? Thank you!

- feat: added support for `debug/toolbar` and `debug/variables/context`, - feat: added support for `debugState` when context (eclipse-theia#11871), - feat: can customize debug session timeout, and error handling (eclipse-theia#11879), - fix: the `debugType` context that is not updated, - fix: `configure` must happen after receiving capabilities (eclipse-theia#11886), - fix: added missing conext menu in the _Variables_ view, - fix: handle `setFunctionBreakboints` response with no `body` (eclipse-theia#11885), - fix: `DebugExt` fires `didStart` event on `didCreate` (eclipse-theia#11916), - fix: validate editor selection based on the text model (eclipse-theia#11880) Closes eclipse-theia#11871 Closes eclipse-theia#11879 Closes eclipse-theia#11885 Closes eclipse-theia#11886 Closes eclipse-theia#11916 Closes eclipse-theia#11880 Signed-off-by: Akos Kitta <[email protected]>

Signed-off-by: Akos Kitta <[email protected]>

kittaakos · 2023-01-17T08:43:55Z

@marcdumais-work and @JonasHelming, could you please help move this PR forward? We at Arduino would like to get these fixes in Theia. Could you tell me the procedure for contacting other interested parties in the verification? Something like the community did here: #10333. Thank you!

vince-fugnitto · 2023-01-17T17:08:15Z

Press F5, readme.md opens, and the top stack frame is the first line of the readme file. Check the DevTools console; there is no Error: Illegal value for lineNumber. (#11880)

In my case (Ubuntu) pressing F5 will trigger the error in the devConsole, is it expected?

tsmaeder

First reading of the code with some suggestions and questions.

packages/debug/src/browser/debug-session-connection.ts

packages/debug/src/browser/debug-session.tsx

tsmaeder · 2023-01-18T09:21:47Z

packages/debug/src/browser/debug-session.tsx

+    /**
+     * Invoked when sending the `terminate` request to the debugger is rejected or timed out.
+     */
+    protected handleTerminateError(err: unknown): void {


This introduces new API, and I don't really see why. What's the use case?

It will help downstream to customize the error handling without overriding the entire stop method and copy-pasting code from Theia to downstream. This generally simplifies the Theia version bumps for extenders.

What's the use case?

I can add dummy customization to the api-samples extension.

packages/debug/src/browser/view/debug-toolbar-widget.tsx

packages/editor/src/browser/editor.ts

CHANGELOG.md

kittaakos · 2023-01-18T10:03:29Z

Thank you!

In my case (Ubuntu) pressing F5 will trigger the error in the devConsole, is it expected?

No. It does not. I see that the vscode-mock-debug sends 0 for the column although columnsStartAt1 is true. These checks will always be false, as the Position properties must be unsigned integers:

https://github.com/kittaakos/theia/blob/76f93cb0d9c2e7e79d5dc943094bc2c8a0eff027/packages/editor/src/browser/editor-manager.ts#L256-L257

For the time being, I have opened an issue and will provide a workaround against the negative integer properties: microsoft/vscode-mock-debug#85.

Signed-off-by: Akos Kitta <[email protected]>

kittaakos · 2023-01-18T11:37:07Z

Thanks a lot for the help! I pushed some changes based on the API review and also added a workaround for the Illegal value for lineNumber error mentioned in #11984 (comment).

The collection of contributed commands was written in a convoluted way, this commit makes it more straightforward with just 2 loops.

The ternary implementation is stricly equivalent to the `Math.max` function, so use that instead.

Assigning a default option object to set default values is problematic as said default values will be discarded if any option object is passed. Instead default values must be handled in conjuction of whatever options are passed to functions.

paul-marechal

Code now LGTM after some rewrites, changes work as described.

kittaakos · 2023-01-19T14:27:26Z

Code now LGTM after some rewrites

:) Thanks for the help

kittaakos · 2023-01-19T14:41:55Z

@thegecko, would you be interested in checking if this PR does not break anything with your debug adapter as you did here: #10333 (review). Thank you!

thegecko · 2023-01-19T15:43:12Z

@thegecko, would you be interested in checking if this PR does not break anything with your debug adapter as you did here: #10333 (review). Thank you!

I can

thegecko · 2023-01-19T16:48:14Z

OK, browser-based debug has been broken in the latest (1.33.0 release). I need to investigate this before checking your PR.

thegecko

I have tested this changeset hasn't broken Arm's browser debugger, but haven't tested the additional functionality.

thegecko · 2023-01-20T12:36:20Z

By the way @kittaakos please reach out if you want any information with testing the browser-based arm debugger yourself:

https://open-vsx.org/extension/arm/embedded-debug

tsmaeder · 2023-01-20T13:27:47Z

@thegecko would it be possible to design some tests that would break when we break front end debugging? Or are the breakages to diverse?

thegecko · 2023-01-20T14:04:53Z

thegecko would it be possible to design some tests that would break when we break front end debugging? Or are the breakages to diverse?

I was thinking about this. One key thing which often breaks browser debugging is accidental inclusion of node libraries. I'd hope we can mitigate that at build time with the correct webpack configs.

I wonder if a simple inline debug adapter test harness could be created (perhaps based on mock-debug) which could be used to test the UI in browser mode?

msujew · 2023-01-24T12:44:54Z

@paul-marechal Do you plan on merging this before the release?

paul-marechal · 2023-01-24T15:20:47Z

@msujew I was waiting on @thegecko to confirm this change doesn't make things worse for their use cases, LGTM now.

- revert debug feature patching (eclipse-theia/theia#11984) - revert menu ordering hack (eclipse-theia/theia#8377) Signed-off-by: Akos Kitta <[email protected]>

kittaakos force-pushed the #11871 branch 3 times, most recently from 15fdf2a to 90f99b0 Compare December 13, 2022 16:45

kittaakos marked this pull request as ready for review December 13, 2022 16:47

colin-grant-work reviewed Dec 13, 2022

View reviewed changes

packages/debug/src/browser/view/debug-toolbar-widget.tsx Outdated Show resolved Hide resolved

packages/debug/src/browser/view/debug-toolbar-widget.tsx Outdated Show resolved Hide resolved

vince-fugnitto added vscode issues related to VSCode compatibility debug issues that related to debug functionality labels Dec 14, 2022

kittaakos mentioned this pull request Dec 16, 2022

vscode: add support for debug/toolbar, debug/variables/context menus contribution and debugState when clause context #11871

Closed

Akos Kitta added 3 commits January 17, 2023 09:40

fix: use when context for command node filtering

e15e5cd

Signed-off-by: Akos Kitta <[email protected]>

chore: removed test debug VSIX

76f93cb

Signed-off-by: Akos Kitta <[email protected]>

kittaakos force-pushed the #11871 branch from 1b2c9c9 to 76f93cb Compare January 17, 2023 08:40

kittaakos requested a review from colin-grant-work January 17, 2023 09:49

vince-fugnitto requested review from vince-fugnitto and tsmaeder January 17, 2023 14:11

tsmaeder reviewed Jan 18, 2023

View reviewed changes

fix: revert timeout check

e10d75e

Signed-off-by: Akos Kitta <[email protected]>

colin-grant-work removed their request for review January 18, 2023 10:38

Akos Kitta added 8 commits January 18, 2023 11:41

fix: clarification on the possible 'debugState'

963cc3d

Signed-off-by: Akos Kitta <[email protected]>

fix: use hard-coded debugger clientID and clientName

57e7f39

Signed-off-by: Akos Kitta <[email protected]>

fix: use review-requested method name

2b6e114

Signed-off-by: Akos Kitta <[email protected]>

fix: changed method name + added doc

96954d2

Signed-off-by: Akos Kitta <[email protected]>

fix: stopTimeout is a default ctor argument

7d99556

Signed-off-by: Akos Kitta <[email protected]>

fix: incorrect method name

84b752d

Signed-off-by: Akos Kitta <[email protected]>

fix: both didCreate and didStart must be API

2b79c77

Signed-off-by: Akos Kitta <[email protected]>

fix: call both on create and start

4e5057c

Signed-off-by: Akos Kitta <[email protected]>

fix: workaround for microsoft/vscode-mock-debug#85

804c93f

Signed-off-by: Akos Kitta <[email protected]>

paul-marechal added 3 commits January 19, 2023 05:02

simplify writing

3a8e329

The collection of contributed commands was written in a convoluted way, this commit makes it more straightforward with just 2 loops.

use Math.max to clamp values into positives

3f07478

The ternary implementation is stricly equivalent to the `Math.max` function, so use that instead.

fix default option value handling

d6e57ab

Assigning a default option object to set default values is problematic as said default values will be discarded if any option object is passed. Instead default values must be handled in conjuction of whatever options are passed to functions.

paul-marechal approved these changes Jan 19, 2023

View reviewed changes

thegecko self-requested a review January 19, 2023 16:20

thegecko approved these changes Jan 20, 2023

View reviewed changes

paul-marechal merged commit 678e335 into eclipse-theia:master Jan 24, 2023

vince-fugnitto added this to the 1.34.0 milestone Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: various debug fixes and VS Code compatibility enhancements #11984

fix: various debug fixes and VS Code compatibility enhancements #11984

kittaakos commented Dec 13, 2022 •

edited

Loading

kittaakos commented Jan 6, 2023

kittaakos commented Jan 17, 2023

vince-fugnitto commented Jan 17, 2023

tsmaeder left a comment

tsmaeder Jan 18, 2023

kittaakos Jan 18, 2023 •

edited

Loading

kittaakos commented Jan 18, 2023

kittaakos commented Jan 18, 2023

paul-marechal left a comment

kittaakos commented Jan 19, 2023

kittaakos commented Jan 19, 2023

thegecko commented Jan 19, 2023

thegecko commented Jan 19, 2023

thegecko left a comment

thegecko commented Jan 20, 2023

tsmaeder commented Jan 20, 2023

thegecko commented Jan 20, 2023

msujew commented Jan 24, 2023

paul-marechal commented Jan 24, 2023

fix: various debug fixes and VS Code compatibility enhancements #11984

fix: various debug fixes and VS Code compatibility enhancements #11984

Conversation

kittaakos commented Dec 13, 2022 • edited Loading

What it does

How to test

For reviewers:

Review checklist

Reminder for reviewers

kittaakos commented Jan 6, 2023

kittaakos commented Jan 17, 2023

vince-fugnitto commented Jan 17, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder Jan 18, 2023

Choose a reason for hiding this comment

kittaakos Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

kittaakos commented Jan 18, 2023

kittaakos commented Jan 18, 2023

paul-marechal left a comment

Choose a reason for hiding this comment

kittaakos commented Jan 19, 2023

kittaakos commented Jan 19, 2023

thegecko commented Jan 19, 2023

thegecko commented Jan 19, 2023

thegecko left a comment

Choose a reason for hiding this comment

thegecko commented Jan 20, 2023

tsmaeder commented Jan 20, 2023

thegecko commented Jan 20, 2023

msujew commented Jan 24, 2023

paul-marechal commented Jan 24, 2023

kittaakos commented Dec 13, 2022 •

edited

Loading

kittaakos Jan 18, 2023 •

edited

Loading